Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of creating an Access Policy for a non-existent Key Vault #2922

Merged
merged 2 commits into from
Feb 21, 2019
Merged

Fix handling of creating an Access Policy for a non-existent Key Vault #2922

merged 2 commits into from
Feb 21, 2019

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented Feb 20, 2019

This pull request adds an acceptance test demonstrating the case where a key vault access policy resource is created with a bad key vault ID. Currently, the Terraform CLI gives the following output for this configuration:

resource "azurerm_key_vault_access_policy" "policy" {
  # This ID shouldn't exist but must be in the URI-style format with enough components
  key_vault_id = "/subscriptions/redacted/resourceGroups/myrg/providers/Microsoft.KeyVault/vaults/idontexist"

  tenant_id = "00000000-0000-0000-0000-000000000000"
  object_id = "11111111-1111-1111-1111-111111111111"

  key_permissions = [
    "get",
  ]

  secret_permissions = [
    "get",
  ]
}
$ terraform apply                                                                                                                                               ⇡ master :: 19h :: ⬡

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  + azurerm_key_vault_access_policy.policy
      id:                   <computed>
      key_permissions.#:    "1"
      key_permissions.0:    "get"
      key_vault_id:         "/subscriptions/redacted/resourceGroups/myrg/providers/Microsoft.KeyVault/vaults/idontexist"
      object_id:            "11111111-1111-1111-1111-111111111111"
      resource_group_name:  <computed>
      secret_permissions.#: "1"
      secret_permissions.0: "get"
      tenant_id:            "00000000-0000-0000-0000-000000000000"
      vault_name:           <computed>


Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

azurerm_key_vault_access_policy.policy: Creating...
  key_permissions.#:    "" => "1"
  key_permissions.0:    "" => "get"
  key_vault_id:         "" => "/subscriptions/redacted/resourceGroups/myrg/providers/Microsoft.KeyVault/vaults/idontexist"
  object_id:            "" => "11111111-1111-1111-1111-111111111111"
  resource_group_name:  "" => "<computed>"
  secret_permissions.#: "" => "1"
  secret_permissions.0: "" => "get"
  tenant_id:            "" => "00000000-0000-0000-0000-000000000000"
  vault_name:           "" => "<computed>"
azurerm_key_vault_access_policy.policy: Creation complete after 0s

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

However, state shows:

{
    "version": 3,
    "terraform_version": "0.11.11",
    "serial": 1,
    "lineage": "74a73fbb-d75a-af7a-c1dc-6d73cc1e9a73",
    "modules": [
        {
            "path": [
                "root"
            ],
            "outputs": {},
            "resources": {},
            "depends_on": []
        }
    ]
}

The test reproduces this workflow, and the subsequent commit makes it pass by removing the branch at which a nil state and nil error is returned during creation.

This commit adds an acceptance test reproducing the scenario of attempting to
create an `azurerm_key_vault_access_policy` resource when specifying the ID of a
Key Vault which does not exist, but is in the correct URI-path-style format. It
generates these by appending the string "NOPE" to the ID of a valid Key Vault
(which the tests creates), but could equally truncate the name (which is how
this was encountered in the first place).

Applying this particular configuration, I expect an error - probably of the form
"Error retrieving Key Vault" (but the exact text is arbitrary). Instead, we see
that no error is produced.

If this scenario is run via the CLI, the CLI marks the resource as successfully
created, but no state is created for it.
@ghost ghost added the size/M label Feb 20, 2019
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jen20,

Thank you for the bugfix. While this works great on creation, on update we still need to check for a not found response as the keyvault the policy was created with no longer exists (and thus by proxy the resource has been deleted)

This will also need to be done for the other key vault resources but i'll open a PR for that.

This fixes the test added in the previous commit, while retaining the ability to
reflect access policy deletion by deleting the key vault itself.
@jen20
Copy link
Contributor Author

jen20 commented Feb 21, 2019

Thanks @katbyte - after e99279b, the related tests all run OK:

$ make testacc TESTARGS='-run=TestAccAzureRMKeyVaultAccessPolicy'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMKeyVaultAccessPolicy -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
?   	github.com/terraform-providers/terraform-provider-azurerm	[no test files]
=== RUN   TestAccAzureRMKeyVaultAccessPolicy_basic
=== PAUSE TestAccAzureRMKeyVaultAccessPolicy_basic
=== RUN   TestAccAzureRMKeyVaultAccessPolicy_basicClassic
=== PAUSE TestAccAzureRMKeyVaultAccessPolicy_basicClassic
=== RUN   TestAccAzureRMKeyVaultAccessPolicy_requiresImport
--- SKIP: TestAccAzureRMKeyVaultAccessPolicy_requiresImport (0.00s)
    resource_arm_key_vault_access_policy_test.go:72: Skipping since resources aren't required to be imported
=== RUN   TestAccAzureRMKeyVaultAccessPolicy_multiple
=== PAUSE TestAccAzureRMKeyVaultAccessPolicy_multiple
=== RUN   TestAccAzureRMKeyVaultAccessPolicy_update
=== PAUSE TestAccAzureRMKeyVaultAccessPolicy_update
=== RUN   TestAccAzureRMKeyVaultAccessPolicy_nonExistentVault
=== PAUSE TestAccAzureRMKeyVaultAccessPolicy_nonExistentVault
=== CONT  TestAccAzureRMKeyVaultAccessPolicy_basic
=== CONT  TestAccAzureRMKeyVaultAccessPolicy_update
=== CONT  TestAccAzureRMKeyVaultAccessPolicy_multiple
=== CONT  TestAccAzureRMKeyVaultAccessPolicy_nonExistentVault
=== CONT  TestAccAzureRMKeyVaultAccessPolicy_basicClassic
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_nonExistentVault (193.92s)
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_basicClassic (203.56s)
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_multiple (206.81s)
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_basic (210.38s)
--- PASS: TestAccAzureRMKeyVaultAccessPolicy_update (221.80s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	221.840s
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/azure	0.025s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/kubernetes	0.014s [no tests to run]
?   	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/response	[no test files]
?   	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/set	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/suppress	0.031s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/tf	0.020s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/validate	0.020s [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils	0.031s [no tests to run]
?   	github.com/terraform-providers/terraform-provider-azurerm/version	[no test files]

@katbyte katbyte added this to the 1.23.0 milestone Feb 21, 2019
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @jen20, LGTM 👍

@katbyte katbyte merged commit 68da8c5 into hashicorp:master Feb 21, 2019
katbyte added a commit that referenced this pull request Feb 21, 2019
@jen20 jen20 deleted the jen20/key-vault-access-policy branch February 21, 2019 19:17
@ghost
Copy link

ghost commented Mar 8, 2019

This has been released in version 1.23.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
	version = "~> 1.23.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Mar 24, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants